Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add original Request to the Response type. #464

Merged
merged 1 commit into from
Apr 21, 2021
Merged

Add original Request to the Response type. #464

merged 1 commit into from
Apr 21, 2021
+22 −1

Conversation

miguelclean
Copy link
Contributor

Another proposal to get the original Request within managerModifyResponse.

Example usage:

main :: IO ()
main = do
  manager <- newManager defaultManagerSettings { managerModifyResponse = logRsp }
  request <- parseRequest "http://httpbin.org/get"
  response <- httpLbs request manager
  print $ responseBody response
  where
    logRsp :: Response a -> IO (Response a)
    logRsp response = do
      putStrLn $  "Response to request: " <> show (path $ responseOriginalRequest response) <> " = "
                                          <> show (responseStatus response)
      pure response

@snoyberg
Copy link
Owner

I think I'm on board with this one, and I like the naming. It strongly implies that this is the request before any redirects are followed. Some considerations:

  • Before merging, this should include a minor version bump and changelog entry (I'm guessing you were already planning that based on the @since waiting to be filled in)
  • To avoid confusion with the request body, perhaps replacing the request body with an empty one would be a Good Thing, with that condition documented
  • Instead of directly exposing the accessors, I'd rather expose a function getOriginalRequest or similar, so that it can be used for setting values. I realize that's not how the rest of the fields work, but it's how I wish I'd written them

@miguelclean
Copy link
Contributor Author

Thank you for the positive response. I just updated this pull request accordingly, however I'm still not sure if it meets the expectations.

In particular I was not sure where to place the getOriginalRequest function. Beside that, now its documenation seems to duplicate the (non-public) documentation of the responseOriginalRequest field itself.

@snoyberg
Copy link
Owner

It actually looks perfect to me. Only final request I'd make is to add a comment on responseOriginalRequest explaining that it's intentionally not exported, and that we export getOriginalRequest instead.

@miguelclean
Copy link
Contributor Author

Glad to hear that. I updated the comment on responseOriginalRequest.

snoyberg
snoyberg approved these changes Apr 21, 2021
View reviewed changes
Copy link
Owner

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you!

@snoyberg snoyberg merged commit c8ee1a9 into snoyberg:master Apr 21, 2021
avieth added a commit to avieth/http-client that referenced this pull request Jul 18, 2022
@avieth
remove original request from Response
eeae3d5 
Deliver it instead by way of the Manager's response modifier.

See snoyberg#464

This patch accomplishes the same thing as that one in essentially the
same way, but improves upon it by not requiring a request to exist in
order to construct a response. This can be useful in e.g. testing /
mocking definitions in which we want to express a base fixed response.
In other wrods, it is "less breaky" and easier to adopt.
@avieth avieth mentioned this pull request Jul 18, 2022
Open
@avieth
Copy link

avieth commented Jul 18, 2022

I know this is already released so it's maybe a fool's errand, but I want to propose this alternative implementation which doesn't require having a Request in order to make a Response value #488

avieth added a commit to avieth/http-client that referenced this pull request Jul 18, 2022
@avieth
remove original request from Response
30c6bbb 
Deliver it instead by way of the Manager's response modifier.

See snoyberg#464

This patch accomplishes the same thing as that one in essentially the
same way, but improves upon it by not requiring a request to exist in
order to construct a response. This can be useful in e.g. testing /
mocking definitions in which we want to express a base fixed response.
In other wrods, it is "less breaky" and easier to adopt.
avieth added a commit to avieth/http-client that referenced this pull request Nov 3, 2023
@avieth
remove original request from Response
beadedb 
Deliver it instead by way of the Manager's response modifier.

See snoyberg#464

This patch accomplishes the same thing as that one in essentially the
same way, but improves upon it by not requiring a request to exist in
order to construct a response. This can be useful in e.g. testing /
mocking definitions in which we want to express a base fixed response.
In other wrods, it is "less breaky" and easier to adopt.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snoyberg snoyberg snoyberg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@miguelclean @snoyberg @avieth